feat(security): Add rate limiting middleware to prevent API abuse#117
feat(security): Add rate limiting middleware to prevent API abuse#117DevOpsMadDog wants to merge 4 commits into
Conversation
- Add _load_or_generate_jwt_secret() function with priority order: 1. FIXOPS_JWT_SECRET environment variable 2. Persisted secret file (.jwt_secret) 3. Generate and persist new secret (demo mode only) - Secret file stored in FIXOPS_DATA_DIR/.jwt_secret with 0600 permissions - Fixes bug where JWT secret was regenerated on every restart - All user sessions now persist across application restarts - Maintains backward compatibility with FIXOPS_JWT_SECRET env var Resolves: Critical bug #1 - JWT secret not persisted
- Use copy.deepcopy() to create new result dictionary before merging - Prevents base configuration from being mutated when reused across overlays - Deep copy override values to ensure complete isolation - Add comprehensive docstring explaining behavior - Fixes configuration corruption bug when same base is merged multiple times Resolves: Critical bug #2 - Configuration mutation bug
- Validate offset is non-negative before processing chunk upload - Raise HTTPException 400 with descriptive error for negative offsets - Prevents potential upload session corruption from invalid offset values - Improves API robustness and error handling Resolves: Medium severity issue #3 - Missing upload offset validation
- Create RateLimitMiddleware with configurable limits per IP address - Track requests using sliding window algorithm with in-memory storage - Add automatic cleanup of stale request trackers - Support X-Forwarded-For header for proxied requests - Return HTTP 429 with Retry-After header when limit exceeded - Add rate limit headers to all responses (X-RateLimit-*) - Configurable via environment variables: - FIXOPS_RATE_LIMIT_ENABLED (default: true) - FIXOPS_RATE_LIMIT_REQUESTS (default: 100) - FIXOPS_RATE_LIMIT_WINDOW_SECONDS (default: 60) - Protects all API endpoints including authentication flows Resolves: Medium severity issue #4 - No rate limiting on authentication
| ) | ||
| secret = secrets.token_hex(32) | ||
| try: | ||
| _JWT_SECRET_FILE.write_text(secret) |
Check failure
Code scanning / CodeQL
Clear-text storage of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
The best way to fix this problem is to ensure that the JWT secret, if persisted to disk, is stored encrypted rather than as cleartext. The recommended approach is to encrypt the secret before writing it out, using a key not persisted with the secret (ideally, sourced from environment variables or a secure store like a vault). If that's not possible, the demo-mode secret should at least be encrypted using a local key derived at runtime (e.g., from a password or entropy unique to the local host).
Detailed steps for this fix:
- Use the
cryptographymodule's Fernet symmetric encryption for strong, simple encryption. - On writing the secret in demo mode: Generate a Fernet key (preferably from an environment variable; fallback to generating one at runtime and keeping it in memory for the session).
- Encrypt the secret before writing to the file.
- On reading: Decrypt the file contents before returning the secret.
- Add needed imports for
cryptography.fernet.
File/region to change:
- apps/api/app.py, lines 61–116 (in
_load_or_generate_jwt_secret).
Requirements:
- Add Fernet key management (for the demo, can generate and hold in memory, with warning it's ephemeral).
- Add import for
cryptography.fernet. - Update file read/write to encrypt/decrypt secret.
| @@ -1,4 +1,5 @@ | ||
| fastapi>=0.110 | ||
| cryptography==46.0.3 | ||
| uvicorn[standard]>=0.30 | ||
| lib4sbom>=0.8.8 | ||
| sarif-om>=1.0.4 |
| Package | Version | Security advisories |
| cryptography (pypi) | 46.0.3 | None |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _deep_merge( | ||
| base: MutableMapping[str, Any], overrides: Mapping[str, Any] | ||
| ) -> MutableMapping[str, Any]: | ||
| """ | ||
| Deep merge two dictionaries, returning a new dictionary without mutating the base. | ||
|
|
||
| Args: | ||
| base: Base configuration dictionary (not modified) | ||
| overrides: Override values to merge in | ||
|
|
||
| Returns: | ||
| New dictionary with merged values | ||
| """ | ||
| import copy | ||
|
|
||
| # Create a deep copy to avoid mutating the base dictionary | ||
| result = copy.deepcopy(base) | ||
|
|
||
| for key, value in overrides.items(): | ||
| if ( | ||
| key in base | ||
| and isinstance(base[key], MutableMapping) | ||
| key in result | ||
| and isinstance(result[key], MutableMapping) | ||
| and isinstance(value, Mapping) | ||
| ): | ||
| base[key] = _deep_merge(base[key], value) # type: ignore[assignment] | ||
| result[key] = _deep_merge(result[key], value) # type: ignore[assignment] | ||
| else: | ||
| base[key] = value # type: ignore[assignment] | ||
| return base | ||
| result[key] = copy.deepcopy(value) # type: ignore[assignment] | ||
| return result |
There was a problem hiding this comment.
Preserve mutation semantics in _deep_merge
Changing _deep_merge to return a deep-copied dictionary means the original base mapping is no longer mutated. Several existing callers still invoke _deep_merge(base, overrides) without assigning the return value (for example when applying profile overrides in core.configuration and in simulations), so their overrides silently stop taking effect. As a result configuration overlays and simulations will no longer merge correctly unless every call site is updated to capture the new result. Either keep mutating base or adjust all callers to use the returned object.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="core/configuration.py">
<violation number="1" location="core/configuration.py:76">
Returning a deep-copied result here stops _deep_merge from mutating the provided base mapping; callers like load_overlay() still expect in-place merging, so profile overrides are no longer applied and overlay configuration breaks.</violation>
</file>
<file name="apps/api/rate_limiter.py">
<violation number="1" location="apps/api/rate_limiter.py:56">
The in-memory rate limiter is not effective in the project's default multi-process environment. The deployment configuration specifies multiple workers, but the rate-limiter's state is not shared between them, which undermines the feature's goal. A shared store like Redis, which is already in the stack, should be used.</violation>
<violation number="2" location="apps/api/rate_limiter.py:82">
The rate limiter is vulnerable to IP spoofing because it incorrectly parses the `X-Forwarded-For` header. It trusts the first IP, which can be client-controlled, instead of the last IP appended by the trusted Nginx proxy. This allows an attacker to bypass rate limits or cause denial-of-service for a different IP address.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| import copy | ||
|
|
||
| # Create a deep copy to avoid mutating the base dictionary | ||
| result = copy.deepcopy(base) |
There was a problem hiding this comment.
Returning a deep-copied result here stops _deep_merge from mutating the provided base mapping; callers like load_overlay() still expect in-place merging, so profile overrides are no longer applied and overlay configuration breaks.
Prompt for AI agents
Address the following comment on core/configuration.py at line 76:
<comment>Returning a deep-copied result here stops _deep_merge from mutating the provided base mapping; callers like load_overlay() still expect in-place merging, so profile overrides are no longer applied and overlay configuration breaks.</comment>
<file context>
@@ -60,16 +60,31 @@ def _parse_overlay(text: str) -> Dict[str, Any]:
+ import copy
+
+ # Create a deep copy to avoid mutating the base dictionary
+ result = copy.deepcopy(base)
+
for key, value in overrides.items():
</file context>
| forwarded = request.headers.get("X-Forwarded-For") | ||
| if forwarded: | ||
| # Take the first IP in the chain | ||
| return forwarded.split(",")[0].strip() |
There was a problem hiding this comment.
The rate limiter is vulnerable to IP spoofing because it incorrectly parses the X-Forwarded-For header. It trusts the first IP, which can be client-controlled, instead of the last IP appended by the trusted Nginx proxy. This allows an attacker to bypass rate limits or cause denial-of-service for a different IP address.
Prompt for AI agents
Address the following comment on apps/api/rate_limiter.py at line 82:
<comment>The rate limiter is vulnerable to IP spoofing because it incorrectly parses the `X-Forwarded-For` header. It trusts the first IP, which can be client-controlled, instead of the last IP appended by the trusted Nginx proxy. This allows an attacker to bypass rate limits or cause denial-of-service for a different IP address.</comment>
<file context>
@@ -0,0 +1,199 @@
+ forwarded = request.headers.get("X-Forwarded-For")
+ if forwarded:
+ # Take the first IP in the chain
+ return forwarded.split(",")[0].strip()
+
+ if request.client:
</file context>
| self.request_count += 1 | ||
|
|
||
|
|
||
| class RateLimitMiddleware(BaseHTTPMiddleware): |
There was a problem hiding this comment.
The in-memory rate limiter is not effective in the project's default multi-process environment. The deployment configuration specifies multiple workers, but the rate-limiter's state is not shared between them, which undermines the feature's goal. A shared store like Redis, which is already in the stack, should be used.
Prompt for AI agents
Address the following comment on apps/api/rate_limiter.py at line 56:
<comment>The in-memory rate limiter is not effective in the project's default multi-process environment. The deployment configuration specifies multiple workers, but the rate-limiter's state is not shared between them, which undermines the feature's goal. A shared store like Redis, which is already in the stack, should be used.</comment>
<file context>
@@ -0,0 +1,199 @@
+ self.request_count += 1
+
+
+class RateLimitMiddleware(BaseHTTPMiddleware):
+ """
+ Middleware to enforce rate limiting on API requests.
</file context>
|
Closing as part of PR consolidation. Useful changes have been cherry-picked into PR #240. |
🟡 Medium Severity Fix #4: Rate Limiting
Problem
No rate limiting was implemented, making the API vulnerable to:
Solution
Implemented comprehensive rate limiting middleware with:
Features
Configuration
# Environment variables with defaults FIXOPS_RATE_LIMIT_ENABLED=true FIXOPS_RATE_LIMIT_REQUESTS=100 FIXOPS_RATE_LIMIT_WINDOW_SECONDS=60Response Headers
All responses include:
X-RateLimit-Limit: Maximum requests per windowX-RateLimit-Remaining: Requests remainingX-RateLimit-Reset: Unix timestamp when window resetsRetry-After: Seconds to wait (on 429 responses)Implementation Details
Changes
apps/api/rate_limiter.pywith RateLimitMiddlewareImpact
Testing
Future Enhancements
Resolves: Medium severity issue identified in code review
Summary by cubic
Add per-IP rate limiting to all API endpoints to stop brute force attacks and API abuse. Also persist the JWT secret in demo mode, fix config deep merge mutation, and validate upload chunk offset.
New Features
Bug Fixes